pkg/aiusechat/uctypes/usechat-types.go (1)

419-427: Return the slice element’s address, not the range copy.

&tool points at the loop variable copy, so any mutation on the returned pointer won’t flow back to opts.Tools/opts.TabTools. Please take the address of the slice element instead (e.g. &opts.Tools[i]) to preserve shared state.

-	for _, tool := range opts.Tools {
-		if tool.Name == toolName {
-			return &tool
-		}
-	}
+	for i := range opts.Tools {
+		if opts.Tools[i].Name == toolName {
+			return &opts.Tools[i]
+		}
+	}
-	for _, tool := range opts.TabTools {
-		if tool.Name == toolName {
-			return &tool
-		}
-	}
+	for i := range opts.TabTools {
+		if opts.TabTools[i].Name == toolName {
+			return &opts.TabTools[i]
+		}
+	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

pkg/aiusechat/tools_readfile.go (1)

27-58: Hoist the shared default line-count constant

DefaultLineCount is still defined inside this helper while readTextFileCallback redeclares it and ToolInputDesc hard-codes 100. This keeps the duplication that was called out earlier and risks the two paths diverging. Please promote the constant to package scope and reuse it everywhere (parser, callback, and descriptor).

-const StopReasonMaxBytes = "max_bytes"
+const (
+	StopReasonMaxBytes = "max_bytes"
+	DefaultLineCount   = 100
+)

-func parseReadTextFileInput(input any) (*readTextFileParams, error) {
-	const DefaultLineCount = 100
-
+func parseReadTextFileInput(input any) (*readTextFileParams, error) {

-	count := 100
+	count := DefaultLineCount

-	if origin == "start" && offset == 0 && count == 100 {
+	if origin == "start" && offset == 0 && count == DefaultLineCount {

pkg/aiusechat/openai/openai-backend.go (1)

833-848: Populate ToolDesc even without ToolInputDesc.

We still ship blank descriptions for every tool that lacks ToolInputDesc, so the new tool-use panel shows an empty body. Initialize ToolDesc from toolDef.Desc() before attempting the specialized override, and only replace it when the callback returns a non-empty string.

 func createToolUseData(toolCallID, toolName string, toolDef *uctypes.ToolDefinition, arguments string) *uctypes.UIMessageDataToolUse {
 	toolUseData := &uctypes.UIMessageDataToolUse{
 		ToolCallId: toolCallID,
 		ToolName:   toolName,
 		Status:     uctypes.ToolUseStatusPending,
 	}
 
 	if toolDef == nil {
 		toolUseData.Status = uctypes.ToolUseStatusError
 		toolUseData.ErrorMessage = "tool not found"
 		return toolUseData
 	}
 
+	toolUseData.ToolDesc = toolDef.Desc()
+
 	var parsedArgs any
 	if err := json.Unmarshal([]byte(arguments), &parsedArgs); err != nil {
 		toolUseData.Status = uctypes.ToolUseStatusError
 		toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err)
 		return toolUseData
 	}
 
 	if toolDef.ToolInputDesc != nil {
-		toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs)
+		if desc := toolDef.ToolInputDesc(parsedArgs); desc != "" {
+			toolUseData.ToolDesc = desc
+		}
 	}

pkg/aiusechat/uctypes/usechat-types.go (1)

429-440: Return pointers to the real ToolDefinition entries.

Using for _, tool := range ... { return &tool } hands back a pointer to the loop variable copy; any later mutation (e.g., toggling approval callbacks) won’t touch the struct stored in Tools / TabTools. Iterate by index so you return the actual slice element.

-	for _, tool := range opts.Tools {
-		if tool.Name == toolName {
-			return &tool
-		}
-	}
-	for _, tool := range opts.TabTools {
-		if tool.Name == toolName {
-			return &tool
-		}
-	}
+	for i := range opts.Tools {
+		if opts.Tools[i].Name == toolName {
+			return &opts.Tools[i]
+		}
+	}
+	for i := range opts.TabTools {
+		if opts.TabTools[i].Name == toolName {
+			return &opts.TabTools[i]
+		}
+	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

pkg/aiusechat/openai/openai-backend.go (1)

829-858: Populate ToolDesc even when ToolInputDesc is absent.

ToolDesc remains empty unless the tool defines ToolInputDesc, leaving most tools without a description in the UI. Initialize toolUseData.ToolDesc with a default from the tool definition (e.g., toolDef.Description or toolDef.ShortDescription) immediately after retrieving toolDef on line 836, then optionally override it with the input-specific description from ToolInputDesc if parsing succeeds.

Apply this diff to set a default description:

 	if toolDef == nil {
 		toolUseData.Status = uctypes.ToolUseStatusError
 		toolUseData.ErrorMessage = "tool not found"
 		return toolUseData
 	}
+	
+	// Set default description from tool definition
+	if toolDef.ShortDescription != "" {
+		toolUseData.ToolDesc = toolDef.ShortDescription
+	} else if toolDef.Description != "" {
+		toolUseData.ToolDesc = toolDef.Description
+	}
 
 	var parsedArgs any
 	if err := json.Unmarshal([]byte(arguments), &parsedArgs); err != nil {
 		toolUseData.Status = uctypes.ToolUseStatusError
 		toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err)
 		return toolUseData
 	}
 
 	if toolDef.ToolInputDesc != nil {
-		toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs)
+		if desc := toolDef.ToolInputDesc(parsedArgs); desc != "" {
+			toolUseData.ToolDesc = desc
+		}
 	}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

pkg/aiusechat/tools_readfile.go (2)

28-61: Consolidate defaults and complete validation in parser.

Avoid duplicating DefaultLineCount and add missing checks (origin, count>=1, normalize negative offset). Centralize default for consistency.

- func parseReadTextFileInput(input any) (*readTextFileParams, error) {
-   const DefaultLineCount = 100
+ const DefaultLineCount = 100
+
+ func parseReadTextFileInput(input any) (*readTextFileParams, error) {
    result := &readTextFileParams{}
@@
-  if result.Count == nil {
-    count := DefaultLineCount
-    result.Count = &count
-  }
-
-  return result, nil
+  if result.Count == nil {
+    count := DefaultLineCount
+    result.Count = &count
+  }
+  if *result.Count < 1 {
+    return nil, fmt.Errorf("count must be at least 1, got %d", *result.Count)
+  }
+  if *result.Origin != "start" && *result.Origin != "end" {
+    return nil, fmt.Errorf("invalid origin value %q: must be 'start' or 'end'", *result.Origin)
+  }
+  if *result.Offset < 0 {
+    zero := 0
+    result.Offset = &zero
+  }
+  return result, nil

From past review comments

---

249-278: Use DefaultLineCount in ToolInputDesc (avoid hardcoded 100).

-  count := 100
+  count := DefaultLineCount
   if parsed.Count != nil {
     count = *parsed.Count
   }

From past review comments

pkg/aiusechat/openai/openai-backend.go (1)

829-858: Initialize ToolDesc with default value before checking ToolInputDesc.

Currently, ToolDesc remains empty unless ToolInputDesc is defined (lines 849-851). Tools without ToolInputDesc will ship a blank description to the UI, undermining the richer status display this PR introduces. Initialize ToolDesc with toolDef.Desc() immediately after the nil check (line 836), then override it if ToolInputDesc provides a more specific description.

Apply this diff to provide a default description:

 func createToolUseData(toolCallID, toolName string, toolDef *uctypes.ToolDefinition, arguments string) *uctypes.UIMessageDataToolUse {
 	toolUseData := &uctypes.UIMessageDataToolUse{
 		ToolCallId: toolCallID,
 		ToolName:   toolName,
 		Status:     uctypes.ToolUseStatusPending,
 	}
 
 	if toolDef == nil {
 		toolUseData.Status = uctypes.ToolUseStatusError
 		toolUseData.ErrorMessage = "tool not found"
 		return toolUseData
 	}
 
+	toolUseData.ToolDesc = toolDef.Desc()
+
 	var parsedArgs any
 	if err := json.Unmarshal([]byte(arguments), &parsedArgs); err != nil {
 		toolUseData.Status = uctypes.ToolUseStatusError
 		toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err)
 		return toolUseData
 	}
 
 	if toolDef.ToolInputDesc != nil {
-		toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs)
+		if desc := toolDef.ToolInputDesc(parsedArgs); desc != "" {
+			toolUseData.ToolDesc = desc
+		}
 	}
 
 	if toolDef.ToolApproval != nil {
 		toolUseData.Approval = toolDef.ToolApproval(parsedArgs)
 	}
 
 	return toolUseData
 }

pkg/util/ds/syncmap.go (1)

55-64: Document that testFn executes under the map lock.

testFn runs while holding the mutex. If it blocks or re-enters SyncMap, it can deadlock. Add a doc comment advising testFn be fast/non‑blocking and not call back into SyncMap.

pkg/aiusechat/usechat.go (1)

306-316: Minor: avoid defers inside loop for cleanup (optional).

Deferring Delete per iteration is fine for small N; alternatively collect IDs and delete in one deferred function to reduce defer overhead.

- for _, toolCall := range stopReason.ToolCalls {
-   activeToolMap.Set(toolCall.ID, true)
-   defer activeToolMap.Delete(toolCall.ID)
- }
+ var ids []string
+ for _, c := range stopReason.ToolCalls {
+   activeToolMap.Set(c.ID, true)
+   ids = append(ids, c.ID)
+ }
+ defer func() {
+   for _, id := range ids {
+     activeToolMap.Delete(id)
+   }
+ }()

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

pkg/aiusechat/tools_readfile.go (3)

28-61: Add missing validation to complete the function.

The previous critical issue with the double pointer (line 37) has been fixed. However, the validation gaps identified in the earlier review remain:

1. Missing origin validation (compare to lines 145-147): No check that origin is "start" or "end"
2. Missing count validation (compare to lines 157-159): No check that count >= 1
3. Missing offset normalization (compare to lines 162-164): Negative offsets should be normalized to 0

Without these checks, parseReadTextFileInput cannot serve as a complete validation function, limiting its reusability and creating inconsistency with readTextFileCallback.

Apply this diff to add the missing validation:

 	if result.Count == nil {
 		count := DefaultLineCount
 		result.Count = &count
 	}
+	if *result.Count < 1 {
+		return nil, fmt.Errorf("count must be at least 1, got %d", *result.Count)
+	}
+
+	if *result.Origin != "start" && *result.Origin != "end" {
+		return nil, fmt.Errorf("invalid origin value '%s': must be 'start' or 'end'", *result.Origin)
+	}
+
+	if *result.Offset < 0 {
+		offset := 0
+		result.Offset = &offset
+	}
 
 	return result, nil
 }

---

29-29: Extract duplicate constant to module level.

DefaultLineCount is defined in both parseReadTextFileInput (line 29) and readTextFileCallback (line 88). This duplication creates a maintenance risk.

Apply this diff to define the constant once at module level:

 const StopReasonMaxBytes = "max_bytes"
+const DefaultLineCount = 100
 
 type readTextFileParams struct {

Then remove the local definitions:

 func parseReadTextFileInput(input any) (*readTextFileParams, error) {
-	const DefaultLineCount = 100
-
 	result := &readTextFileParams{}

 func readTextFileCallback(input any) (any, error) {
-	const DefaultLineCount = 100
 	const DefaultMaxBytes = 50 * 1024
 	const ReadLimit = 1024 * 1024 * 1024

Also applies to: 88-88

---

249-278: Replace hardcoded value with constant.

The ToolInputDesc implementation provides helpful human-readable descriptions. However, line 263 uses the hardcoded value 100 instead of referencing DefaultLineCount, creating a maintenance issue if the default changes.

Apply this diff after extracting DefaultLineCount to module level:

-			count := 100
+			count := DefaultLineCount
 			if parsed.Count != nil {
 				count = *parsed.Count
 			}
 
-			if origin == "start" && offset == 0 {
+			if origin == "start" && offset == 0 && count == DefaultLineCount {
 				return fmt.Sprintf("reading %q (first %d lines)", parsed.Filename, count)
 			}
-			if origin == "end" && offset == 0 {
+			if origin == "end" && offset == 0 && count == DefaultLineCount {
 				return fmt.Sprintf("reading %q (last %d lines)", parsed.Filename, count)
 			}

frontend/app/aipanel/aimessage.tsx (1)

76-101: Fix null safety and add state reset for tool call changes.

The component has two critical issues:

1. toolData properties are accessed without null checking (lines 80-88, 95). If part.data is null—which past comments indicate can happen mid-stream—the component will crash when accessing toolData.status, toolData.approval, or toolData.toolcallid.

2. userApprovalOverride never resets when the tool call changes. If the component instance is reused for a different tool call, the stale override persists, potentially showing "denied" for a brand-new tool and stopping keep-alive emissions.

Based on past review comments

Apply this diff to add null safety and reset logic:

 const AIToolUse = memo(({ part }: AIToolUseProps) => {
     const toolData = part.data;
     const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null);
+    const toolCallId = toolData?.toolcallid;
+    const effectiveApproval = userApprovalOverride ?? toolData?.approval ?? "";
 
-    const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•";
-    const statusColor =
-        toolData.status === "completed"
-            ? "text-green-400"
-            : toolData.status === "error"
-              ? "text-red-400"
-              : "text-gray-400";
-
-    const effectiveApproval = userApprovalOverride || toolData.approval;
-
     useEffect(() => {
-        if (effectiveApproval !== "needs-approval") return;
+        if (!toolCallId || effectiveApproval !== "needs-approval") {
+            return;
+        }
 
         const interval = setInterval(() => {
             RpcApi.WaveAIToolApproveCommand(TabRpcClient, {
-                toolcallid: toolData.toolcallid,
+                toolcallid: toolCallId,
                 keepalive: true,
             });
         }, 4000);
 
         return () => clearInterval(interval);
-    }, [effectiveApproval, toolData.toolcallid]);
+    }, [effectiveApproval, toolCallId]);
+
+    useEffect(() => {
+        setUserApprovalOverride(null);
+    }, [toolCallId, toolData?.approval]);
+
+    if (!toolData) {
+        return null;
+    }
+
+    const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•";
+    const statusColor =
+        toolData.status === "completed"
+            ? "text-green-400"
+            : toolData.status === "error"
+              ? "text-red-400"
+              : "text-gray-400";

frontend/app/aipanel/aimessage.tsx (1)

73-73: Remove unused prop.

The isStreaming prop is declared but never used in the component.

Apply this diff to remove the unused prop:

 interface AIToolUseProps {
     part: WaveUIMessagePart & { type: "data-tooluse" };
-    isStreaming: boolean;
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

pkg/aiusechat/tools_readfile.go (3)

28-29: Extract duplicate constant to package level.

DefaultLineCount is defined in both parseReadTextFileInput (line 29) and readTextFileCallback (line 100), creating a maintenance issue if the default value needs to change.

Apply this diff to define the constant once at package level:

 package aiusechat

 import (
 	"fmt"
 	"io"
 	"os"
 	"strings"

 	"github.com/wavetermdev/waveterm/pkg/aiusechat/uctypes"
 	"github.com/wavetermdev/waveterm/pkg/util/readutil"
 	"github.com/wavetermdev/waveterm/pkg/util/utilfn"
 	"github.com/wavetermdev/waveterm/pkg/wavebase"
 )

+const DefaultLineCount = 100
 const StopReasonMaxBytes = "max_bytes"

Then remove the duplicate definitions from both functions:

 func parseReadTextFileInput(input any) (*readTextFileParams, error) {
-	const DefaultLineCount = 100
-
 	result := &readTextFileParams{}

 func readTextFileCallback(input any) (any, error) {
-	const DefaultLineCount = 100
 	const DefaultMaxBytes = 50 * 1024

---

28-73: Past review comment remains unaddressed: readTextFileCallback should use this function.

The past review comment suggested refactoring readTextFileCallback to use parseReadTextFileInput internally to eliminate duplication. Currently, readTextFileCallback (lines 99-217) still contains its own validation logic, duplicating the work done here.

Consider refactoring readTextFileCallback to use this function:

 func readTextFileCallback(input any) (any, error) {
 	const DefaultMaxBytes = 50 * 1024
 	const ReadLimit = 1024 * 1024 * 1024

-	var params readTextFileParams
-	if err := utilfn.ReUnmarshal(&params, input); err != nil {
-		return nil, fmt.Errorf("invalid input format: %w", err)
+	params, err := parseReadTextFileInput(input)
+	if err != nil {
+		return nil, err
 	}

-	if params.Filename == "" {
-		return nil, fmt.Errorf("missing filename parameter")
-	}
-
 	maxBytes := DefaultMaxBytes
 	if params.MaxBytes != nil {
 		maxBytes = *params.MaxBytes
 	}

Then remove the duplicate validation logic from lines 152-176 since parseReadTextFileInput already handles it.

---

261-290: Replace hardcoded values with DefaultLineCount constant.

Lines 275 and 280 use the hardcoded value 100 instead of the DefaultLineCount constant, creating a maintenance issue if the default changes. This was flagged in a previous review comment.

Apply this diff to use the constant:

 		ToolInputDesc: func(input any) string {
 			parsed, err := parseReadTextFileInput(input)
 			if err != nil {
 				return fmt.Sprintf("error parsing input: %v", err)
 			}

 			origin := "start"
 			if parsed.Origin != nil {
 				origin = *parsed.Origin
 			}
 			offset := 0
 			if parsed.Offset != nil {
 				offset = *parsed.Offset
 			}
-			count := 100
+			count := DefaultLineCount
 			if parsed.Count != nil {
 				count = *parsed.Count
 			}

-			if origin == "start" && offset == 0 {
+			if origin == "start" && offset == 0 && count == DefaultLineCount {
 				return fmt.Sprintf("reading %q (first %d lines)", parsed.Filename, count)
 			}
 			if origin == "end" && offset == 0 {
 				return fmt.Sprintf("reading %q (last %d lines)", parsed.Filename, count)
 			}
 			if origin == "end" {
 				return fmt.Sprintf("reading %q (from end: offset %d lines, count %d lines)", parsed.Filename, offset, count)
 			}
 			return fmt.Sprintf("reading %q (from start: offset %d lines, count %d lines)", parsed.Filename, offset, count)
 		},

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro